Address PR feedback from #2579 - #2611
Conversation
We don't need a TryConvert method, we already have a TryParse, which can be used instead.
| int csrc = 0; | ||
| if (!Conversions.Instance.TryConvert(in spanT, ref csrc) || csrc <= 0) | ||
| int csrc; | ||
| if (!Conversions.Instance.TryParse(in spanT, out csrc) || csrc <= 0) |
There was a problem hiding this comment.
Note that TryParse is going to be slightly more expensive, but it's unlikely to be measurable, especially given everything else going on here.
There was a problem hiding this comment.
slightly more expensive
Because of the extra write to the out parameter? Or because of the under/overflow check? Or something else?
There was a problem hiding this comment.
The extra indirect write in the case of success, two extra indirect writes in the case of failure, the overflow check, the cast from the nullable rather than GetValueOrDefault, larger code that's even less likely to be inlined (though it's probably already not getting inlined). In general this code is not particularly clean.
|
This begs the question why the methods are named Convert and TryParse, rather than Convert and TryConvert, or Parse and TryParse. As long as you're cleaning things up, seems it should be consistent. |
Codecov Report
@@ Coverage Diff @@
## master #2611 +/- ##
==========================================
+ Coverage 71.53% 71.53% +<.01%
==========================================
Files 800 800
Lines 141866 141857 -9
Branches 16122 16121 -1
==========================================
- Hits 101478 101475 -3
+ Misses 35937 35931 -6
Partials 4451 4451
|
|
@stephentoub - I've logged #2669 for your concerns on the existing |
ghost
locked as resolved and limited conversation to collaborators
5 participants
We don't need a TryConvert method, we already have a TryParse, which can be used instead.